Conversation
597b52a to
6d74645
Compare
6d1d6b1 to
1c7d1cb
Compare
|
Thank you for merging #30 @tehrengruber , @havogt . |
b5f514e to
3281313
Compare
|
I only did a brief walk through the code so far. One question popped up and I didn't follow the code to figure it out myself: how is the version managed? Is it taking always the version of atlas? |
atlas4py version is not managed by the found atlas library, but that could be a strategy. I did not deviate from what was in place. I removed however duplicate places where the version was defined with only a single place remaining, in the There is still the extra variable |
3281313 to
7ef5085
Compare
|
Note, just rebased again on master. |
| PYBIND11_MODULE( _atlas4py, m ) { | ||
| m.def("_initialise", atlasInitialise) | ||
| .def("_finalise", atlas::finalise); | ||
| m.attr("version") = atlas::Library::instance().version(); |
There was a problem hiding this comment.
| m.attr("version") = atlas::Library::instance().version(); | |
| m.attr("atlas_version") = atlas::Library::instance().version(); |
or just removing it altogether?
| m.attr("version") = atlas::Library::instance().version(); |
There was a problem hiding this comment.
I suppose in time, __version__ and version/atlas_versionshould be the same. We might as well remove it then indeed.
| else() | ||
| set(rpath_origin_install_libdir "$ORIGIN/${CMAKE_INSTALL_LIBDIR}") | ||
| endif() | ||
| set( CMAKE_INSTALL_RPATH "${rpath_origin_install_libdir}" ) # A semicolon-separated list specifying the RPATH to use in installed targets |
There was a problem hiding this comment.
Can you add a motivation for the 3 additional options here?
There was a problem hiding this comment.
Consider the use case without these added lines, and where we have atlas detected and not being built.
I then have this error at runtime (for instance for macOS):
from ._atlas4py import *
E ImportError: dlopen(/path/to/venv/lib/python3.13/site-packages/atlas4py/_atlas4py.cpython-313-darwin.so, 0x0002): Library not loaded: @rpath/libatlas.0.45.dylib
It is clear that the rpath of the found atlas libraries is not added. CMake provides the option
set( CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE ) # add the automatic parts to RPATH which point to dirs outside build tree
to automatically add the rpaths of libraries which are not built within the build-tree, introspected from the location of the library. This then adds the rpath to the external atlas library. We could probably guard this with if (build_atlas) to make sure no extra rpaths are inserted accidentally.
The next options probably do not play a role because it is more for a standard package with multiple libraries and executables involving running executables from within the build-tree and saving time on installation. But it is the good practice that we always set by default for all our software.
set( CMAKE_SKIP_BUILD_RPATH FALSE ) # use RPATHs for the build tree
When CMAKE_SKIP_BUILD_RPATH is set to FALSE, then the executables in the build-tree etc. are also built with RPATHS.
set( CMAKE_BUILD_WITH_INSTALL_RPATH TRUE ) # build with *relative* rpaths by default
That means that the build-tree executables have been built with the same rpaths we encoded for the install. Usually we have compile destinations within the build-tree in a similar structure as install: <build>/lib, <build>/bin, and the rpaths are relative "$ORIGIN/../lib". This saves time on relinking on installation, and is then just a copy.
We could omit these last two but it is good practice, and possibly speeds up installs.
tehrengruber
left a comment
There was a problem hiding this comment.
Looks good, thanks!
| set(rpath_origin_install_libdir "$ORIGIN/${CMAKE_INSTALL_LIBDIR}") | ||
| endif() | ||
| set( CMAKE_INSTALL_RPATH "${rpath_origin_install_libdir}" ) # A semicolon-separated list specifying the RPATH to use in installed targets | ||
| set( CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE ) # add the automatic parts to RPATH which point to dirs outside build tree |
There was a problem hiding this comment.
| set( CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE ) # add the automatic parts to RPATH which point to dirs outside build tree | |
| if (NOT build_atlas) | |
| set( CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE ) # add external atlas to rpath | |
| endif() |
Thanks for the explanation. The if around the CMAKE_INSTALL_RPATH_USE_LINK_PATH makes sense to me, even if it is just for readability.
7472b50 to
08602bd
Compare
|
Thanks @tehrengruber I have followed your last recommendations and cleaned up the history. I can only see the "Squash and merge" option for merging. Perhaps it would be useful to keep the individual commits (e.g "Rebase and merge"). Could you take care of this? |
I have made time available this year to work on our plan to extend atlas4py and hand over maintenance.
As a first step I tried the new build-system, and noticed that I needed to tweak the installation a little bit.